Conversation
sejoon00
left a comment
There was a problem hiding this comment.
고생하셨어요!
기능에 대한 테스트 코드는 swagger로 수행했다고 생각할게요!
시간이 촉박하니 간단하게 수정할 수 있는 것만 고치고 merge하고 제가 말한 부분은 고민하고 추후에 수정해봐요!
There was a problem hiding this comment.
problems/all/ 이런 방식이 괜찮을 것 같아요
URL을 디렉토리 구조라고 생각했을때 문제 도메인에서 전체문제를 찾는 위치를 들어가는 느낌이 괜찮지않나 싶네요
There was a problem hiding this comment.
좋은것 같습니다! 도메인을 타고 들어간다면 problem/all 은 어떨까요?
There was a problem hiding this comment.
Client 이름이 너무 방대한 것 같아요
Client 안에서도 쪼개는게 좋을 것 같아요.
해설, 문제 이런것들도 client안에서 각 controller로 나누는게 좋은 것 같습니다
나중에 멀티모듈로 쪼개면서 각 모듈에서는 이름이 겹쳐도 되니까 쪼개면서 나눠봐요
There was a problem hiding this comment.
좋은 것 같아요.
일단 문제랑 해설 2개로 분리해놓겠습니다!
There was a problem hiding this comment.
간단한 리뷰인데요! Reurn으로 내보낼거라면 else if 안쓰고
if (currentStatus == CORRECT) {
return isCorrect ? CORRECT : INCORRECT;
}
if (currentStatus == INCORRECT) {
return isCorrect ? RETRY_CORRECT : INCORRECT;
}
if (currentStatus == IN_PROGRESS) {
return isCorrect ? CORRECT : INCORRECT;
}
return Comment;처럼 하고 틀렸는지 맞았는지 로직도 메서드 추출하면 가독서잉 좀더 좋아질것 같아요
아니면 switch문도 괜찮은것 같습니다
There was a problem hiding this comment.
그렇네요! switch문으로 변경하겠습니다.
There was a problem hiding this comment.
파라미터 개수가 많아져서 도메인을 넘겨서 Dto로 바꿔주는 mapper로직을 DTO안에 구성하면 좋을 것 같아요
There was a problem hiding this comment.
이 친구도 mapper 로직이 DTO로 들어가면 좋겠네용
There was a problem hiding this comment.
상수로 처리하는 것도 괜찮지만 달 계산하는 로직은 많이 쓴다면 util처럼 감싸두 되구용
| List<ProblemSubmit> submissions = problemSubmitRepository.findByMemberIdAndPublishId(memberId, publishId); | ||
| List<ProblemSubmitStatus> problemStatuses = submissions.stream() | ||
| .map(ProblemSubmit::getStatus) | ||
| .toList(); |
There was a problem hiding this comment.
요새 저의 코드도 그렇고 특정 조건으로 도메인을 조회하는 repository가 여러 service레이어에서 사용되면서 의존성이 점점 퍼지고 얽혀서 나중에 분리하기가 어려워질 것 같다는 생각이 드는데요.
특정 도메인 조회하는 서비스레이어를 거쳐서 가져오는것도 괜찮다는 생각이 드는 요즘입니다.
There was a problem hiding this comment.
좋습니다. 특히 조회 로직 작성할 때 의존성이 점점 불어나더라고요. 리팩토링 단계에서 같이 이야기해봐요!
💡 Issue
📄 Description
💬 To Reviewers